feat(jira): add --jira-trailer flag to extract Jira issue key from git trailer - #1109
Open
vidhu-balad wants to merge 16 commits into
Open
feat(jira): add --jira-trailer flag to extract Jira issue key from git trailer#1109vidhu-balad wants to merge 16 commits into
vidhu-balad wants to merge 16 commits into
Conversation
…iler When --jira-trailer <key> is set, the command reads lines of the form '<key>: <value>' from the commit message and uses those values as the sole source of Jira issue references, skipping the full commit message and branch name scan. This avoids false positives from other trailers (e.g. Ona-Environment-Id) whose values happen to match the Jira key pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Claude finished @vidhu-balad's task in 4m 25s —— View job Review — PR #1109
|
| Check | Result |
|---|---|
go build ./... |
clean |
go vet ./... |
clean |
go mod tidy + git diff --exit-code go.mod go.sum |
clean |
I could not run the test suites in this environment: the command-test suites need the local Kosli server (make test_integration, docker compose), and go test ./internal/gitview/ was refused by the sandbox. Test correctness below is by reading, not by execution — please confirm make test_integration_single TARGET=AttestJiraCommandTestSuite and go test ./internal/gitview/... locally.
Phase 2 — findings
Two, both minor; nothing blocking. Posted inline:
attestJira.go:358— the bare-trailer warning prints an empty list. A commit withJira:and no value yieldstrailer 'Jira' was found but contained no valid Jira issue keys: [](pinned by test 32).trailerValuesis empty by construction there, so the[]is noise and "contained" is inaccurate —TrailerKeyExistsis exactly what tells the two cases apart, so one extra branch gives each its own sentence. Same warning, second input: with--jira-project-key ABCandJira: EX-1,EX-1is filtered on the project key and reported as "no valid Jira issue keys", which points the user at the wrong fix.gitView.go:289— a blank key degrades to a":"prefix instead of matching nothing, in bothTrailerKeyExistsandGetTrailerValues. Unreachable fromattest jirathanks to the newPreRunEguard, but these are exported functions in a shared package whose contract doesn't mention it; two early returns make the package self-consistent.
Things I specifically checked and found correct:
- Audit spec is complete.
jira-traileris in bothflags_to_testandflag_valuesinhack/empty-flag-audit/spec.jsonand incmd/kosli/testdata/empty-flag-audit-coverage.json, sorequire_full_coveragewon'tSystemExit.jira-secondary-sourcelives only inflag_values, not the always-setflagsdict, so the new mutual exclusion can't collide with the--jira-trailerprobe run — that was the risk here and it's clear. - The Unicode slice fix is right.
strings.ToLower("İ")is 1 byte against 2 in the source, so the oldtrimmed[len(prefix):]would have returned": BX-123"; theIndexByterewrite givesBX-123, and the test atgitView_test.go:565pins it. - The two notions of "set" now agree.
PreRunErejects every blank-ish--jira-trailer, sotrailerKey != ""inrun()can no longer diverge fromflag.Changed— the gating inconsistency from earlier rounds is closed. --assertmessages follow the mode.issueSourcethreads through both error paths, so trailer mode no longer tells the user to look in the commit body and branch it deliberately skipped.- CRLF is handled (
TrimSpaceon each line), the help text no longer over-claims that the flag "bypasses pattern-scanning", and theLong/example/flag-help prose now agrees with the mutual-exclusion validation.
Phase 3 — what's good here
The test matrix (27–35) is genuinely strong: test 27 uses --assert with a decoy Ona-Environment-Id: ONA-999 so it fails in both directions, and test 35 puts EX-1 in the branch name so branch scanning leaking into trailer mode would be caught. That's the headline claim of the PR pinned properly rather than a golden that passes vacuously. Extracting NormalizeTrailerKey so validation and matching can't drift apart is the right call, and the İ case shows real care about the byte-offset assumption. The audit spec + coverage json being updated together is the part that's easiest to forget and it's done.
· branch feat/jira-trailer-flag
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
@vidhu-balad there is still some feedback from the bot |
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…curate help - Trim leading whitespace from trailer lines so editor-indented or git-log-formatted messages (4-space indent) match correctly - Strip trailing colon from --jira-trailer value so "Jira:" and "Jira" both produce the same prefix - Error when --jira-trailer and --jira-secondary-source are both set (they are mutually exclusive; secondary source is silently ignored in trailer mode) - Warn when --ignore-branch-match is set alongside --jira-trailer (it has no effect in trailer mode) - Warn when a trailer is found but contains no valid Jira issue keys - Thread issueSource through both --assert error messages so trailer mode names the trailer rather than "commit message or branch name" - Update Long description to document trailer mode, its interaction with --ignore-branch-match, and its use as the preferred CVE-collision fix - Add --jira-trailer example to attestJiraExample Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…canned test - TrimSpace the key in GetTrailerValues before TrimRight(key, ":") - Add unit test: key with surrounding whitespace still matches - Add integration test 30: --jira-trailer does not scan branch name Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…exclusion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Drop dangling "of the form" from long desc opening line - Replace "are not scanned" with explicit mutual-exclusion statement - Replace "bypasses pattern-scanning entirely" with scoped claim - Add "Mutually exclusive with --jira-trailer" to jiraSecondarySourceFlag Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… value
- Validate that --jira-trailer collapses to a non-empty key after
TrimSpace+TrimRight(":"), preventing silent non-compliance
- Add integration test 31 pinning the blank-ish key error path
- Rename old test 31 → 32
- Add unit test: line with empty value is skipped in GetTrailerValues
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gn error wording
- Extract gitview.NormalizeTrailerKey to deduplicate TrimSpace+TrimRight(":")
used in both GetTrailerValues and attestJira validation
- Fix GetTrailerValues to slice by colon index rather than len(lowercased prefix),
avoiding wrong offset when key's lowercase form has different byte length
- Update GetTrailerValues doc comment: document empty-value drop, any-line
semantics, and key normalisation behaviour
- Align blank-ish --jira-trailer error to repo-wide wording:
"flag '--jira-trailer' was given an empty value"
- Add unit test pinning the Unicode key case
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add gitview.TrailerKeyExists to detect a key line regardless of whether its value is empty, complementing GetTrailerValues which skips empty values - Update warning condition to use TrailerKeyExists so a bare "Jira:" line now triggers "trailer found but contained no valid Jira issue keys" - Update GetTrailerValues doc comment to reference TrailerKeyExists - Add unit tests for TrailerKeyExists Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Apply NormalizeTrailerKey once at the top of the trailer block so that tolerated flag forms (trailing colon, surrounding whitespace) appear in their canonical form in debug logs, warnings, and --assert error messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add test 32: trailer present with non-Jira value triggers the "trailer found but no valid Jira issue keys" warning. Rename old 32 → 33. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add test 32: bare Jira: line triggers TrailerKeyExists warning path - Rename old 32/33 → 33/34 - Fix example comment: replace "bypasses...entirely" with scoped claim matching the long desc caveat (trailer value still pattern-matched) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r to audit spec - Add test 34: --ignore-branch-match warns it has no effect in trailer mode - Rename old 34 → 35 - Add jira-trailer to flags_to_test and flag_values in empty-flag-audit spec.json Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--jira-trailer <key>flag tokosli attest jira<key>: <value>from the commit message and uses those values as the sole source of Jira issue referencesOna-Environment-Id: ONA-456) whose values happen to match the Jira key patternChanges
internal/gitview/gitView.go— newGetTrailerValues(message, key string) []stringfunctioninternal/gitview/gitView_test.go— 6 unit tests covering no match, single match, case-insensitive key, multiple occurrences, non-matching trailers ignored, whitespace trimmingcmd/kosli/root.go—jiraTrailerFlagconstantcmd/kosli/attestJira.go—--jira-trailerflag wired into the issue-finding logiccmd/kosli/attestJira_test.go— 3 integration tests: trailer used successfully, trailer absent (non-compliant but reported), trailer absent with--assert(error)Test plan
make test_integration_single TARGET=AttestJiraCommandTestSuite— tests 27, 28, 29 cover the new flaggo test ./internal/gitview/... -run TestGitViewTestSuite/TestGetTrailerValues— unit tests forGetTrailerValuesmake lint— passes with 0 issues🤖 Generated with Claude Code